Skip to content

fix: harden redirect validation and bounded reads#3671

Merged
mnriem merged 1 commit into
github:mainfrom
PascalThuet:fix/http-hardening-follow-up
Jul 22, 2026
Merged

fix: harden redirect validation and bounded reads#3671
mnriem merged 1 commit into
github:mainfrom
PascalThuet:fix/http-hardening-follow-up

Conversation

@PascalThuet

Copy link
Copy Markdown
Contributor

Summary

This is a follow-up to #3140, prepared after that PR merged while a final security and robustness audit was still running.

  • reject remote redirects to local targets across canonical, legacy numeric, percent-encoded, IDNA, scoped IPv6, mapped IPv4, and unspecified-address spellings
  • keep HTTP available only for unambiguous loopback URLs used by local development, while preserving normal HTTPS/CDN redirects
  • make bounded response reads memory-stable under adversarial short reads by using 64 KiB reads and a single BytesIO buffer
  • validate malformed redirect ports and unexpected Azure CLI/Azure AD token payload shapes without leaking raw parser exceptions
  • centralize the shared download URL policy used by authentication, presets, and workflows
  • ensure self-upgrade tests exercise the isolated opener used by production code

Root cause and impact

The merged redirect check handled the obvious remote-to-HTTP-loopback case, but validation and connection behavior could still disagree for alternative hostname spellings. It also allowed a remote HTTPS URL to enter a local HTTPS target. In environments that consume attacker-controlled URLs, that gap could enable a conditional SSRF-style read from a service on the local machine or CI runner.

The bounded reader assumed read(size) would normally return large chunks. A response producing one-byte short reads retained every bytes object and then duplicated the payload during join; the 1 MiB reproduction increased RSS by about 94.5 MiB. The new implementation keeps the same byte limit while reducing that reproduction to about 1.6 MiB.

These findings are defense-in-depth rather than a critical vulnerability. Normal remote HTTPS redirects remain supported, and authorization headers are still stripped when a redirect leaves configured trusted hosts.

Compatibility and limits

The redirect policy is deliberately lexical and performs no DNS lookup. DNS or hosts-file rebinding would require connection-level address pinning and is outside this focused follow-up. Raw IPv6 zone syntax is rejected; the RFC %25zone form remains supported.

Validation

  • .venv/bin/python -m pytest: 5,095 passed, 155 skipped
  • uvx ruff check src tests: passed
  • Bandit on the changed source files at medium/high severity: no findings
  • git diff --check: passed

Prepared on behalf of @PascalThuet by OpenAI Codex (model: GPT-5, autonomous).

Assisted-by: OpenAI Codex (model: GPT-5, autonomous)
@PascalThuet
PascalThuet marked this pull request as ready for review July 22, 2026 20:09
@PascalThuet
PascalThuet requested a review from mnriem as a code owner July 22, 2026 20:09
@mnriem
mnriem requested a review from Copilot July 22, 2026 20:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Hardens download redirects, bounded reads, and Azure token parsing across shared HTTP flows.

Changes:

  • Centralizes canonical URL and redirect validation.
  • Makes bounded reads memory-stable with 64 KiB chunks.
  • Adds extensive security and malformed-payload coverage.
Show a summary per file
File Description
src/specify_cli/_download_security.py Implements shared URL policy and bounded buffering.
src/specify_cli/authentication/http.py Applies strict redirect validation.
src/specify_cli/authentication/azure_devops.py Safely extracts token payloads.
src/specify_cli/presets/_commands.py Reuses shared download policy.
src/specify_cli/workflows/_commands.py Reuses shared download policy.
tests/test_download_security.py Covers URL variants and bounded reads.
tests/test_authentication.py Covers redirects and malformed token responses.
tests/test_workflows.py Covers workflow redirect rules.
tests/test_upgrade.py Documents isolated opener testing.
tests/test_self_upgrade_guidance.py Activates opener-routing fixture.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 10/10 changed files
  • Comments generated: 0
  • Review effort level: Medium

@mnriem
mnriem merged commit 3a7a875 into github:main Jul 22, 2026
14 checks passed
@mnriem

mnriem commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants